api: Fix handling of staggering in injection/interpolation by mloubout · Pull Request #2936 · devitocodes/devito

mloubout · 2026-05-20T14:11:05Z

No description provided.

codecov · 2026-05-20T17:24:02Z

Codecov Report

❌ Patch coverage is 99.18831% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.39%. Comparing base (68e2f74) to head (551d050).

Files with missing lines	Patch %	Lines
devito/finite_differences/derivative.py	0.00%	1 Missing and 1 partial ⚠️
tests/test_interpolation.py	99.61%	1 Missing and 1 partial ⚠️
devito/types/sparse.py	96.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2936      +/-   ##
==========================================
+ Coverage   83.33%   83.39%   +0.06%     
==========================================
  Files         248      248              
  Lines       51799    51881      +82     
  Branches     4467     4479      +12     
==========================================
+ Hits        43165    43267     +102     
+ Misses       7882     7861      -21     
- Partials      752      753       +1

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	`68.70% <88.04%> (+0.03%)`	⬆️
pytest-gpu-gcc-	`78.09% <99.18%> (+0.06%)`	⬆️
pytest-gpu-icx-	`78.00% <99.18%> (+0.05%)`	⬆️
pytest-gpu-nvc-nvidiaX	`69.23% <88.04%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

review-notebook-app · 2026-05-21T01:53:53Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini · 2026-05-21T08:13:08Z


+    def _field_shifts(self, field):
+        """
+        Per-grid-Dimension half-cell shift induced by ``field``'s staggering


ultra-nitpick, fixable in a separate PR -- single `, not two, for homogeneity

We seem to be wildly inconsistent on this

FabioLuporini · 2026-05-21T08:13:46Z

+        try:
+            staggered = field.staggered
+        except AttributeError:
+            return None


when can u end up in the AttributeError? add a comment?

FabioLuporini · 2026-05-21T08:14:43Z

+        try:
+            staggered = field.staggered
+        except AttributeError:
+            return None


return empty tuple

FabioLuporini · 2026-05-21T08:15:01Z

+        except AttributeError:
+            return None
+        if not staggered or all(s == 0 for s in staggered):
+            return None


return empty tuple

FabioLuporini · 2026-05-21T08:16:25Z

+        for _, g in groupby(pairs, lambda f: f[0].staggered):
+            g = list(g)
+            g_fields = [f for f, _ in g]
+            g_exprs = [e for _, e in g]


I think you can do

g_fields, g_exprs = zip(*g)

instead of these two lines

FabioLuporini · 2026-05-21T08:21:24Z

+    def _grid_map(self):
+        return {}
+
+    @property


cached_property

FabioLuporini · 2026-05-21T08:21:48Z

+
+    @property
+    def origin(self):
+        return DimensionTuple(*[0]*len(self.dimensions),


ultra-nitpick: self.ndim == len(self.dimensions)

No this is a SparseFunction, self.ndim is the number of grid dimensions, this is the number of sdimension of the SparseFunction

FabioLuporini · 2026-05-21T08:24:44Z

-    def _pos_symbols(self):
-        return [Symbol(name=f'pos{d}', dtype=np.int32)
-                for d in self.grid.dimensions]
+    @memoized_meth


the fact that this had to be turned into a memoized_meth makes me think that it should better be placed in the interpolator classes instead.
Accepting shifts here doesn't make much sense imho.

I think I probably agree with this

This is the symbol for the sparse position. And it's not used only for interpolation, it's used for plain guards such as here that are used for guarding generic sparse expressions based on the position

FabioLuporini · 2026-05-21T08:24:59Z

-    @cached_property
-    def _position_map(self):
+    @memoized_meth
+    def _position_map(self, shifts=None):


same story, imho this method doesn't belong here anymore

It does, this is a SparseFunction information which is it's position relative to an origin. The interpolator uses it, it doesn't define it.

FabioLuporini · 2026-05-21T08:25:26Z

-    "assert np.isclose(norm(rec), 31.23, atol=0, rtol=1e-3)\n",
-    "assert np.isclose(norm(rec2), 3.5482, atol=0, rtol=1e-3)\n",
-    "assert np.isclose(norm(rec3), 4.7007, atol=0, rtol=1e-3)"
+    "assert np.isclose(norm(rec), 29.538, atol=0, rtol=1e-3)\n",


that's quite the change

This one inject the source into v_x which changes things quite a bit over long propagation (2 seconds here) since the source position is not properly nterpolated into the staggered location of v_x

EdCaunt · 2026-05-21T10:21:51Z

+
+    @property
+    def origin(self):
+        return DimensionTuple(*[0]*len(self.dimensions),


Ultra nitpick: DimensionTuple(*[0 for _ in self.dimensions]) would be more readable in this context

EdCaunt · 2026-05-21T10:26:16Z

-    def _pos_symbols(self):
-        return [Symbol(name=f'pos{d}', dtype=np.int32)
-                for d in self.grid.dimensions]
+    @memoized_meth


I think I probably agree with this

EdCaunt · 2026-05-21T10:31:27Z


        # Position map and temporaries for it
-        pmap = self._position_map
+        pmap = self._position_map()


Does this never need to know about the shifts then?

This is a guard for pure sparse operation, there is no staggered origin involved

EdCaunt · 2026-05-21T10:37:22Z

How come this file has been so heavily reworked?

Just reorganized it was a pain to navigate it, it's 99% the same just lifted in classes

mloubout · 2026-05-21T12:09:36Z

+        '(x, y)', '(x, z)', '(y, z)',
+        '(x, y, z)'
+    ])
+    def test_interpolate_staggered(self, stagg):


Note: These three tests are the only new ones in this file

mloubout added 2 commits May 20, 2026 09:38

api: fix interpolation with staggered

7562312

api: fix injection witjh staggering

6552d82

mloubout added the API api (symbolics, types, ...) label May 20, 2026

mloubout force-pushed the inject-interp branch from 3f39935 to 3405f6a Compare May 20, 2026 17:05

mloubout force-pushed the inject-interp branch from 3405f6a to 2001db0 Compare May 21, 2026 01:53

mloubout force-pushed the inject-interp branch 3 times, most recently from bc601dc to 74db52a Compare May 21, 2026 03:08

FabioLuporini reviewed May 21, 2026

View reviewed changes

EdCaunt reviewed May 21, 2026

View reviewed changes

mloubout commented May 21, 2026

View reviewed changes

mloubout force-pushed the inject-interp branch from 74db52a to b9b7bec Compare May 21, 2026 12:40

api: fix inject handling of different staggering

551d050

mloubout force-pushed the inject-interp branch from b9b7bec to 551d050 Compare May 21, 2026 12:49

Conversation

mloubout commented May 20, 2026

Uh oh!

codecov Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

review-notebook-app Bot commented May 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdCaunt May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 20, 2026 •

edited

Loading

EdCaunt May 21, 2026 •

edited

Loading